Skip to content

input and output class refactoring #2483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 2, 2019
Merged

input and output class refactoring #2483

merged 3 commits into from
Feb 2, 2019

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Feb 1, 2019

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2481
License MIT
Doc PR n/a

Alternative to #2481 to get rid of the request attributes and use only the metadata.

TODO:

  • Fix tests

@dunglas dunglas requested a review from soyuka February 1, 2019 11:50
soyuka
soyuka approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good indeed, it's a bit more work then using the request attributes but it'll be cleaner after! Not sure why I didn't do this from scratch :p.

@bendavies bendavies mentioned this pull request Feb 1, 2019
Closed
@ragboyjr
Copy link
Contributor

ragboyjr commented Feb 1, 2019

@dunglas I don't know if this is related to your refactor, but I noticed a few things when testing input classes on 2.4-beta

  1. Input classes were request attributes instead of resource/operation attributes (but it looks like this PR addresses that). Having the input class be on the operation makes sense when you want PUT/POST to be different inputs.
  2. When using input_class, the input class itself needed to be registered as a resource before it could be used. This kind of makes sense only because of how the internals of API Platform works, but it's not clear from the documentation that this is required, and I'm not sure if that was intended. Because it's a resource, you have to run itemOperations: {}, collectionOperations: {} on all your input classes to remove the inputs.
  3. When using messenger with the input classes, you have to specify messenger on the input class definition, and not on the operation metadata. This also makes sense considering how the internals of configuration works, but from a configuration perspective, it feels a bit weird.

Ideally, It'd be great if you could just define the input_class on the operation, API Platform would then default messenger: true since using an input class means you'd need to make a custom data persister or alternatively, use messenger which makes it so much easier to handle and gives you the added benefit that you now have a proper application services for using your app.

App\Entity\Book:
  collectionOperations:
    get: ~
    post:
      # messenger: false (if you want to make a custom data persister)
      input_class: App\DTO\CreateBookRequest

@dunglas
Copy link
Member Author

dunglas commented Feb 1, 2019

@ragboyjr thanks for testing!

  1. should be fixed now
  2. it qualifies at a bug to me
  3. it's weird and shouldn't be the case

@dunglas dunglas changed the base branch from master to 2.4 February 1, 2019 15:54
@ragboyjr
Copy link
Contributor

ragboyjr commented Feb 1, 2019

For 2, and 3, are those things you feel like you want to tackle here, or would you welcome a PR addressing those changes?

@dunglas
Copy link
Member Author

dunglas commented Feb 1, 2019

PRs are very welcome!

@dunglas dunglas force-pushed the better-outputclass branch from d3e9f29 to c3b7d55 Compare February 2, 2019 07:38
@dunglas dunglas merged commit e10d658 into api-platform:2.4 Feb 2, 2019
@dunglas dunglas deleted the better-outputclass branch February 2, 2019 07:57
bendavies
bendavies reviewed Feb 6, 2019
View reviewed changes
src/Bridge/Symfony/Bundle/Resources/config/api.xml
@@ -156,6 +156,7 @@
<service id="api_platform.listener.view.write" class="ApiPlatform\Core\EventListener\WriteListener">
<argument type="service" id="api_platform.data_persister" />
<argument type="service" id="api_platform.iri_converter" on-invalid="null" />
<argument type="service" id="api_platform.metadata.resource.metadata_factory" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing on-invalid="null"

soyuka referenced this pull request in soyuka/core Feb 7, 2019
@soyuka
Missing on-invalid=null https://github.com/api-platform/core/pull/248…
c071930 
…3\#discussion_r254228991
soyuka referenced this pull request in soyuka/core Feb 7, 2019
@soyuka
Missing on-invalid=null https://github.com/api-platform/core/pull/248…
3a63d59 
…3\#discussion_r254228991
soyuka referenced this pull request in soyuka/core Feb 7, 2019
@soyuka
Missing on-invalid=null https://github.com/api-platform/core/pull/248…
fa279d3 
…3\#discussion_r254228991
soyuka pushed a commit to soyuka/core that referenced this pull request Feb 11, 2019
@dunglas @soyuka
input and output class refactoring (api-platform#2483)
5750ced 
* input and output class refactoring

* Fix WriteListener

* Fix unit tests
soyuka referenced this pull request in soyuka/core Feb 11, 2019
@soyuka
Missing on-invalid=null https://github.com/api-platform/core/pull/248…
2407d8b 
…3\#discussion_r254228991
soyuka referenced this pull request in soyuka/core Feb 12, 2019
@soyuka
Missing on-invalid=null https://github.com/api-platform/core/pull/248…
ddabd1a 
…3\#discussion_r254228991
soyuka added a commit to soyuka/core that referenced this pull request Feb 12, 2019
@soyuka
Missing on-invalid=null see api-platform#2483
b14902b
soyuka added a commit to soyuka/core that referenced this pull request Feb 12, 2019
@soyuka
Missing on-invalid=null see api-platform#2483
bb30d60
@norkunas
Copy link
Contributor

Does this supports payload on DELETE request? For example asking user to enter his password before deleting some user related entity.

@soyuka
Copy link
Member

soyuka commented Feb 22, 2019

it should yes. @GregoireHebert worked on a use case where he uses a PUT and did the removal in a messenger handler :D

@norkunas
Copy link
Contributor

Nice then :)

Taluu
Taluu reviewed Feb 23, 2019
View reviewed changes
src/Identifier/ContextAwareIdentifierConverterInterface.php
* @author Antoine Bluchet <soyuka@gmail.com>
*/

namespace ApiPlatform\Core\Identifier;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taluu Taluu Taluu left review comments

@bendavies bendavies bendavies left review comments

@soyuka soyuka soyuka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dunglas @ragboyjr @norkunas @soyuka @Taluu @bendavies